-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add expect_no_lint #2582
Add expect_no_lint #2582
Conversation
Thanks for this. I would suggest adding a NEWS bullet crediting yourself and citing the issue number, as well as updating our test suite to use the new function. |
I would hold this for follow-up actually, that will be several-hundred-lines change, better to keep this PR focused on the minimal change to get this added. To that end we probably want a |
Added NEWS item and yeah the test suit update will be massive. I actually thought about an |
Follow up PR for the migration sounds fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice!
@@ -42,7 +44,8 @@ | |||
expect_lint <- function(content, checks, ..., file = NULL, language = "en") { | |||
if (!requireNamespace("testthat", quietly = TRUE)) { | |||
stop( # nocov start | |||
"'expect_lint' is designed to work within the 'testthat' testing framework, but 'testthat' is not installed.", | |||
"'expect_lint' and 'expect_no_lint' are designed to work within the 'testthat' testing framework, ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally this will just reference the actual call for clarity, but we can save that for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This pull request is meant to address issue #2580.
This is probably the easiest way to add such a function. I assume with time the tests should be updated to use one over the other.